Marquee multi-selection + off-canvas indicators — review

Head: e26eaf81abcadf0329b48c3789714cf11752cb25
Base: fix/runtime-set-tween-resilience (stacked)
Scope: +707/-52 across 10 files. New marqueeGeometry.ts (SAT helper, 12 unit tests), marqueeCommit.ts (drag/commit hook), and off-canvas indicator rendering in DomEditOverlay.tsx.

The SAT machinery and the marquee pointer flow are well-formed. Nice work choosing real OBB intersection over AABB-only — the rotated-square test cases prove it. Two findings worth talking about; nothing blocking.

Finding 1 — PR body / behavior contract drift: "center outside" vs "partially outside" (NIT, doc)

PR description says:

elements with center outside the composition bounds show dashed teal outlines in the gray zone

Implementation in packages/studio/src/components/editor/DomEditOverlay.tsx (off-canvas effect, the partiallyOutside predicate):

const partiallyOutside =
  r.left < compRect.left ||
  r.left + r.width > compRect.left + compRect.width ||
  r.top < compRect.top ||
  r.top + r.height > compRect.top + compRect.height;
if (partiallyOutside) { rects.push(...); }

That fires whenever any edge crosses the composition border — so a 1000×500 element that's mostly inside the comp but pokes 4px past the right edge gets the off-canvas treatment too. The visual is tame here because clipOutside (the polygon(evenodd, ...) cutout) trims away the in-canvas portion of the dashed border, so what the user sees is only the protruding sliver — but the predicate name and the PR contract don't match. Pick one:

• If "any-edge-outside" is the actual intent, update the PR body and rename the local to something like extendsOutsideComp.
• If "center outside" was the intent (centroid-based predicate, matches the natural mental model of "this element lives in the gray zone"), the predicate needs to compute cx = r.left + r.width/2 etc. and test those vs. compRect.

Worth pinning down before merge — it shows up in the test plan too ("Off-canvas elements show dashed indicators" is ambiguous under the current predicate).

Finding 2 — Off-canvas indicator suppression ignores group selection (NIT)

Same file, the render loop for off-canvas indicators:

{offCanvasRects
  .filter((r) => {
    const selEl = selection?.element;
    return !selEl || offCanvasElementsRef.current.get(r.key) !== selEl;
  })
  .map((r) => { ... })}

This suppresses the indicator for the single primary selection.element only — not for the rest of groupSelections. After a marquee multi-select that captures three partially-outside elements, the primary gets the normal selection box and the other two render both the group rect (groupOverlayItems loop above) AND the off-canvas indicator. Two visual layers on the same item.

Also the effect that builds offCanvasRects has dep array [iframeRef, compRect, activeCompositionPath, selection] — it doesn't observe groupSelections, so even if you extend the filter to cover the group, the indicator set won't recompute on group changes until something else triggers it. Fix is both:

1. Extend the filter to walk groupSelections (or build a Set<HTMLElement> once from the active selection set).
2. Add groupSelections (or its derived selection-element set) to the dep array.

Things checked and clean

• SAT correctness: 2 AABB axes + 2 OBB edge normals is the right optimization for two rectangles (parallel sides → one normal per pair). The 45°-rotated-square AABB-overlaps-but-OBB-doesn't test is the right canary. marqueeIntersectsObb handles degenerate edges (len < 1e-9) and zero-width marquees correctly.
• Modifier semantics: Plain marquee → replace; Shift+marquee → add (event.shiftKey threaded through commitMarquee → applyMarqueeSelection). Click without drag (!pastThreshold) → clear via onMarqueeSelectRef.current?.([], false). Matches PR body. No contradictory rules.
• Pointer capture: setPointerCapture on marquee start, releasePointerCapture in onPointerUp + try/catch for already-released. Marquee continues past overlay bounds. Good.
• coversComposition guard in runMarqueeIntersection: full-bleed wrappers correctly excluded from marquee hits.
• clipPath: polygon(evenodd, ...): valid CSS syntax (<fill-rule> is a legal first arg to polygon() per CSS spec).
• No console. introduced*: clean wrt #1691.
• identity-transform fast path in elementObbCorners: skips DOMMatrix walk when transform is identity-ish — sensible perf.

CI

Required checks green at read-time; "Preview parity" + "Graphite / mergeability_check" still in progress (mergeability blocked on downstack which is expected for stacked PRs).

Verdict

NIT — Finding 1 is a contract-clarification ask, Finding 2 is a small visual-double-up that only surfaces with multi-selection of partially-outside elements. Neither blocks merge. The geometry layer is solid and the integration plumbing through applyMarqueeSelection / DomEditActionsContext looks correct.

Review by Via

R1 @ e26eaf81 — Rames D Jusso review

Layering on Via's review (which I've read at HEAD). The SAT geometry, modifier semantics, and pointer-capture flow all check out — agree with everything Via flagged on F1 (partiallyOutside vs PR-body "center outside" contract drift) and F2 (group-selection ignored in off-canvas indicator filter + missing groupSelections dep). These are real and I won't restate them. Below are findings I caught from a different angle.

🛑 — none

⚠️ Concerns

C1 — useDomEditPreviewSync.ts:70-72 silently drops the no-element-found clear

const nextElement = findElementForSelection(doc, currentSelection, activeCompPath);
if (!nextElement) {
-  applyDomSelection(null, { revealPanel: false });
  return;
}

The PR scope is canvas/gray-zone selection plumbing; this hunk is broader — it changes behavior on every preview re-sync where the previously-selected element no longer resolves (composition reload, hot reload, swapping activeCompPath, document replacement after a save). Previously selection was cleared in that case; now domEditSelectionRef.current keeps pointing at a stale HTMLElement from a previous document. The references in boxRef / overlayRect rendering will still attempt geometry on a node that's no longer in the live DOM.

If this was intentional to fix the "drag-off-canvas releases clear selection" symptom mentioned in the PR body, the cleaner fix is in the canvas/box click suppression (you already added suppressNextBoxClickRef for that). Suggest reverting this hunk and adding a comment if there's a specific repro the canvas-path doesn't cover.

C2 — applyMarqueeSelection bypasses STUDIO_INSPECTOR_PANELS_ENABLED and applyDomSelection's side effects

useDomSelection.ts:423-456 implements applyMarqueeSelection as a parallel selection-write path that directly calls setDomEditSelection / setDomEditGroupSelections / setSelectedTimelineElementId — it does not go through applyDomSelection. Two consequences:

1. The if (!STUDIO_INSPECTOR_PANELS_ENABLED) short-circuit in applyDomSelection (line 145) is skipped. If that flag is ever flipped off in prod for a rollback, marquee will still land selections while the inspector panel UI is suppressed — divergent state.
2. The revealPanel / setRightCollapsed(false) / setRightPanelTab("design") behavior is skipped. Marquee multi-select silently leaves the right panel collapsed even when the user just intentionally selected things. Single-click selection reveals the panel; multi-marquee doesn't. Worth verifying intent — could be deliberate ("don't shift the UI on bulk select") or an oversight.

Suggest at minimum honoring the flag check; the panel-reveal is a product call.

C3 — Off-canvas indicator click-target uses clipPath for hit-suppression, which doesn't suppress hit-testing

DomEditOverlay.tsx:589-594. The dashed-indicator div is pointer-events-auto over the FULL bounding rect, with clipPath: polygon(evenodd, ...) visually removing the in-canvas portion. In Chromium, clip-path is purely visual — pointer hit-testing still uses the element's bounding box. So clicking inside the in-canvas portion of an off-canvas element's bounding rect hits this indicator's onClick and the underlying overlay handler at the same coordinates. The e.stopPropagation() inside handleClick prevents the conflict from bubbling, but means a user clicking the in-canvas portion of a partially-off-canvas element always selects via the off-canvas indicator path (which uses skipSourceProbe: true) rather than the normal canvas selection path. Different code path → potentially different DomEditSelection shape for the same element.

Either restrict the overlay's positioned rect to the actually-outside region (compute the outside fragments and create one positioned div per fragment), or accept the duplication and add a comment.

🟡 Nits

N1 — style={marquee.marqueeRef.current?.pastThreshold ? { cursor: "crosshair" } : undefined} reads a ref during render

DomEditOverlay.tsx:410. marqueeRef is a useRef — mutating pastThreshold does not trigger a re-render. The cursor only updates because setMarqueeRect also fires on the same onPointerMove tick, which causes a re-render that happens to re-read the ref. Brittle by construction; if a future refactor delays the setMarqueeRect update or batches it differently, the cursor change desyncs. Promote pastThreshold to a piece of state on the hook return, or read it from a stable indicator like marqueeRect != null.

N2 — Off-canvas indicator is not keyboard-accessible

DomEditOverlay.tsx:589-594. The dashed click target is a <div> with onClick only — no role="button", no tabIndex, no key handler, no aria-label (just title, which screen readers may or may not announce). Studio is a creative tool, so the keyboard-a11y bar is lower than general web, but pure-mouse-only off-canvas reachability means power users can't tab to them. Trivial fix: role="button" tabIndex={0} onKeyDown={(e) => (e.key === "Enter" || e.key === " ") && handleClick(e as any)}.

N3 — Off-canvas effect re-runs on every selection change

DomEditOverlay.tsx:263 dep array [iframeRef, compRect, activeCompositionPath, selection]. Every single-element selection change re-walks collectDomEditLayerItems, computes isElementComputedVisible + toOverlayRect for every item, and re-renders the indicators. With ~50 elements in a composition this is ~50 getBoundingClientRect + getComputedStyle reads per selection change. The dep on selection is only needed to drive the filter (don't show indicator for the selected element); the position computation is independent of selection. Split: one memo computes raw offCanvasRects, a derived list applies the selection filter. Cuts the cost to per-element-list-change.

🟢 What's clean

• SAT machinery + identity-transform fast path in elementObbCorners is right (agree with Via).
• setPointerCapture / releasePointerCapture with try/catch is correct.
• coversComposition guard against full-bleed wrappers.
• runMarqueeIntersection uses skipSourceProbe: true consistently with the off-canvas click handler — same selection shape.
• Hover-rect rendering simplification (dropping the per-element borderRadius computed-style lookup) is fine; the loss in fidelity is minor and the perf win on hover is real.
• 12 marquee geometry tests including the 45°/30° rotated cases and degenerate-OBB edge case — strong coverage.

CI snapshot

Preview-parity ✓, Player perf ✓, regression ✓. Graphite mergeability still in-progress (expected, stacked PR).

— Rames D Jusso

Stamp routing: @rames Jusso once findings addressed (or punted with rationale).